fix: add documented events to menu-button #219 - #232
Conversation
There was a problem hiding this comment.
Pull request overview
Adds documented menu-button-level events for select/change, aligning event listeners with the menu-button widget rather than the underlying menu element.
Changes:
- Adds a dedicated handler for
"makeup-menu-change"and dispatches"makeup-menu-button-change". - Dispatches
"makeup-menu-button-select"on select and refactors collapse/focus logic into a shared helper. - Updates docs example to listen on
widget.elfor the documented menu-button events.
Reviewed changes
Copilot reviewed 2 out of 6 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/ui/makeup-menu-button/src/index.js | Splits change vs select handling, dispatches documented menu-button events, and centralizes collapse behavior. |
| docs/ui/makeup-menu-button/index.js | Updates example listeners to the new menu-button event targets/types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
At first, I wondered why the menu collapses on change. Looking closer, |
Yes, change is traditionally associated with being fired on when an element changes and loses focus, which isn't true here (in the base menu case) - here the change is fired immediately. I'm not sure what else to call it. Any ideas? |
|
@saiponnada I'm just going to move forward with these as named, to be consistent with the docs and to not introduce breaking changes. I do agree though that |
Fixes #219